Skip to content

Fix #863: add ProcessRunner utility to capabilities-common#89

Merged
orpiske merged 1 commit intowanaku-ai:mainfrom
orpiske:ci-issue-863
Mar 3, 2026
Merged

Fix #863: add ProcessRunner utility to capabilities-common#89
orpiske merged 1 commit intowanaku-ai:mainfrom
orpiske:ci-issue-863

Conversation

@orpiske
Copy link
Copy Markdown
Contributor

@orpiske orpiske commented Mar 3, 2026

Summary

  • Add ProcessRunner utility class to capabilities-common module (moved from wanaku core-util)
  • Uses SLF4J logging instead of JBoss Logger
  • Add unit test for runWithOutput()

Test plan

  • mvn verify passes
  • Unit test for runWithOutput() passes

Summary by Sourcery

Introduce a reusable ProcessRunner utility in capabilities-common for executing external processes and capturing their output.

New Features:

  • Add ProcessRunner utility class to capabilities-common for running external processes with optional working directory and environment variables.
  • Provide runWithOutput helper to execute a command and return its combined output as a string.

Tests:

  • Add a unit test validating that runWithOutput returns non-empty output containing the expected content.

@sourcery-ai
Copy link
Copy Markdown

sourcery-ai bot commented Mar 3, 2026

Reviewer's Guide

Adds a reusable ProcessRunner utility to capabilities-common for running external processes with SLF4J logging and introduces a basic unit test for capturing command output.

Sequence diagram for ProcessRunner.runWithOutput execution flow

sequenceDiagram
    participant Client
    participant ProcessRunner
    participant ProcessBuilder
    participant Process
    participant Runtime

    Client->>ProcessRunner: runWithOutput(command)
    activate ProcessRunner
    ProcessRunner->>ProcessBuilder: new ProcessBuilder(command)
    ProcessRunner->>ProcessBuilder: redirectOutput(PIPE), redirectError(PIPE)
    ProcessRunner->>ProcessBuilder: start()
    ProcessBuilder-->>ProcessRunner: Process
    activate Process
    ProcessRunner->>ProcessRunner: readOutput(Process)
    ProcessRunner->>ProcessRunner: waitForExit(Process)
    ProcessRunner->>Runtime: addShutdownHook(Thread)
    Runtime-->>ProcessRunner: hook added
    ProcessRunner->>Process: waitFor()
    Process-->>ProcessRunner: exitCode
    alt exitCode != 0
        ProcessRunner-->>ProcessRunner: log warn
    end
    ProcessRunner->>Runtime: removeShutdownHook(Thread)
    Runtime-->>ProcessRunner: hook removed
    deactivate Process
    ProcessRunner-->>Client: output
    deactivate ProcessRunner
Loading

Class diagram for new ProcessRunner utility

classDiagram
    class ProcessRunner {
        -Logger LOG
        -ProcessRunner()
        +static String runWithOutput(String command)
        +static void run(File directory, String command)
        +static void run(File directory, Map~String,String~ environmentVariables, String command)
        -static String readOutput(Process process)
        -static void waitForExit(Process process)
    }

    class WanakuException {
    }

    class Logger {
    }

    class ProcessBuilder {
    }

    class Process {
    }

    ProcessRunner --> WanakuException : throws
    ProcessRunner --> Logger : uses
    ProcessRunner --> ProcessBuilder : creates
    ProcessBuilder --> Process : starts
    ProcessRunner --> Process : manages
Loading

File-Level Changes

Change Details Files
Introduce ProcessRunner utility for running external processes with optional environment and output capture.
  • Add static runWithOutput API that executes a command, pipes stdout/stderr, collects stdout into a String, and wraps IO/interrupt failures in WanakuException
  • Add overloaded run APIs that execute a command in a given working directory with optional environment variables, inheriting IO and logging command start/finish
  • Implement shutdown hook–backed waitForExit helper that waits for process completion, logs non-zero exit codes, and attempts to destroy the process on JVM shutdown
capabilities-common/src/main/java/ai/wanaku/capabilities/sdk/common/ProcessRunner.java
Add unit test coverage for ProcessRunner.runWithOutput.
  • Add JUnit 5 test that runs echo hello through ProcessRunner.runWithOutput
  • Assert output is non-null, non-empty, and contains the expected token
capabilities-common/src/test/java/ai/wanaku/capabilities/sdk/common/ProcessRunnerTest.java

Tips and commands

Interacting with Sourcery

  • Trigger a new review: Comment @sourcery-ai review on the pull request.
  • Continue discussions: Reply directly to Sourcery's review comments.
  • Generate a GitHub issue from a review comment: Ask Sourcery to create an
    issue from a review comment by replying to it. You can also reply to a
    review comment with @sourcery-ai issue to create an issue from it.
  • Generate a pull request title: Write @sourcery-ai anywhere in the pull
    request title to generate a title at any time. You can also comment
    @sourcery-ai title on the pull request to (re-)generate the title at any time.
  • Generate a pull request summary: Write @sourcery-ai summary anywhere in
    the pull request body to generate a PR summary at any time exactly where you
    want it. You can also comment @sourcery-ai summary on the pull request to
    (re-)generate the summary at any time.
  • Generate reviewer's guide: Comment @sourcery-ai guide on the pull
    request to (re-)generate the reviewer's guide at any time.
  • Resolve all Sourcery comments: Comment @sourcery-ai resolve on the
    pull request to resolve all Sourcery comments. Useful if you've already
    addressed all the comments and don't want to see them anymore.
  • Dismiss all Sourcery reviews: Comment @sourcery-ai dismiss on the pull
    request to dismiss all existing Sourcery reviews. Especially useful if you
    want to start fresh with a new review - don't forget to comment
    @sourcery-ai review to trigger a new review!

Customizing Your Experience

Access your dashboard to:

  • Enable or disable review features such as the Sourcery-generated pull request
    summary, the reviewer's guide, and others.
  • Change the review language.
  • Add, remove or edit custom review instructions.
  • Adjust other review settings.

Getting Help

Copy link
Copy Markdown

@sourcery-ai sourcery-ai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey - I've found 5 issues, and left some high level feedback:

  • In runWithOutput, you redirect stderr to a separate PIPE but never consume it, which risks deadlocking on large error output; consider either redirectErrorStream(true) or reading from both streams.
  • readOutput creates a BufferedReader without closing it; use try-with-resources to ensure the process streams are properly closed after reading.
  • When catching InterruptedException you wrap and rethrow but never restore the interrupt status; consider calling Thread.currentThread().interrupt() before throwing the WanakuException.
Prompt for AI Agents
Please address the comments from this code review:

## Overall Comments
- In `runWithOutput`, you redirect `stderr` to a separate PIPE but never consume it, which risks deadlocking on large error output; consider either `redirectErrorStream(true)` or reading from both streams.
- `readOutput` creates a `BufferedReader` without closing it; use try-with-resources to ensure the process streams are properly closed after reading.
- When catching `InterruptedException` you wrap and rethrow but never restore the interrupt status; consider calling `Thread.currentThread().interrupt()` before throwing the `WanakuException`.

## Individual Comments

### Comment 1
<location path="capabilities-common/src/main/java/ai/wanaku/capabilities/sdk/common/ProcessRunner.java" line_range="44-49" />
<code_context>
+        }
+    }
+
+    private static String readOutput(Process process) throws IOException {
+        // Read the output from the process
+        BufferedReader reader = new BufferedReader(new InputStreamReader(process.getInputStream()));
+        StringBuilder output = new StringBuilder();
+        String line;
+        while ((line = reader.readLine()) != null) {
+            output.append(line).append("\n");
+        }
</code_context>
<issue_to_address>
**suggestion (bug_risk):** Close the BufferedReader/InputStream via try-with-resources to avoid leaking OS resources.

The `BufferedReader` (and underlying `InputStream`) remains open for the lifetime of the process, which can cause resource leaks in long-running or repeated executions. Wrap the reader in a try-with-resources (or close it in a finally block) so it’s always released:

```java
try (BufferedReader reader = new BufferedReader(new InputStreamReader(process.getInputStream()))) {
    StringBuilder output = new StringBuilder();
    String line;
    while ((line = reader.readLine()) != null) {
        output.append(line).append("\n");
    }
    return output.toString();
}
```
</issue_to_address>

### Comment 2
<location path="capabilities-common/src/main/java/ai/wanaku/capabilities/sdk/common/ProcessRunner.java" line_range="24-26" />
<code_context>
+    public static String runWithOutput(String... command) {
+        try {
+            ProcessBuilder processBuilder = new ProcessBuilder(command);
+            // Redirect output and error streams to a pipe
+            processBuilder.redirectOutput(ProcessBuilder.Redirect.PIPE);
+            processBuilder.redirectError(ProcessBuilder.Redirect.PIPE);
+
+            final Process process = processBuilder.start();
</code_context>
<issue_to_address>
**issue (bug_risk):** Reading only stdout while piping stderr risks deadlock for verbose error output.

In `runWithOutput`, both stdout and stderr are piped but only stdout is read. If the subprocess writes heavily to stderr, its buffer can fill and block the process so `waitFor()` never returns. Consider either using `redirectErrorStream(true)` and reading a single stream, or consuming stderr concurrently (e.g., separate thread) / redirecting it to inheritIO or logging instead of a pipe.
</issue_to_address>

### Comment 3
<location path="capabilities-common/src/test/java/ai/wanaku/capabilities/sdk/common/ProcessRunnerTest.java" line_range="11" />
<code_context>
+public class ProcessRunnerTest {
+
+    @Test
+    void runWithOutput_returnsCommandOutput() {
+        String output = ProcessRunner.runWithOutput("echo", "hello");
+
</code_context>
<issue_to_address>
**suggestion (testing):** Add a test for `runWithOutput` when the process exits with a non-zero status

Because `waitForExit` only logs a warning on non-zero exits and doesn’t throw, please add a test that runs a command exiting with status 1 and asserts that `runWithOutput` does not throw and returns the expected output. This will lock in the intended behavior for failure cases and prevent regressions.
</issue_to_address>

### Comment 4
<location path="capabilities-common/src/test/java/ai/wanaku/capabilities/sdk/common/ProcessRunnerTest.java" line_range="10-17" />
<code_context>
+
+    @Test
+    void runWithOutput_returnsCommandOutput() {
+        String output = ProcessRunner.runWithOutput("echo", "hello");
+
+        assertNotNull(output, "Output should not be null");
</code_context>
<issue_to_address>
**suggestion (testing):** The test command may not be fully portable across platforms

Using `echo` as the command makes this test non-portable (e.g., on Windows `echo` is usually a shell built-in, not an executable). To keep the test reliable across CI environments, consider using a known binary available in the test environment (such as `java`) or choosing the command based on the detected OS.

```suggestion
    @Test
    void runWithOutput_returnsCommandOutput() {
        String osName = System.getProperty("os.name").toLowerCase();
        String output;

        if (osName.contains("win")) {
            // On Windows, echo is a cmd.exe built-in
            output = ProcessRunner.runWithOutput("cmd.exe", "/c", "echo hello");
        } else {
            // On Unix-like systems, echo is typically a shell built-in
            output = ProcessRunner.runWithOutput("sh", "-c", "echo hello");
        }

        assertNotNull(output, "Output should not be null");
        assertFalse(output.isEmpty(), "Output should not be empty");
        assertTrue(output.contains("hello"), "Output should contain 'hello'");
    }
```
</issue_to_address>

### Comment 5
<location path="capabilities-common/src/test/java/ai/wanaku/capabilities/sdk/common/ProcessRunnerTest.java" line_range="16" />
<code_context>
+
+        assertNotNull(output, "Output should not be null");
+        assertFalse(output.isEmpty(), "Output should not be empty");
+        assertTrue(output.contains("hello"), "Output should contain 'hello'");
+    }
+}
</code_context>
<issue_to_address>
**suggestion (testing):** Consider adding a test that covers stderr handling in `runWithOutput`

Currently we only cover a stdout-only command. Since `runWithOutput` pipes stderr but only reads stdout, please add a test where the child writes to both streams, and assert that it completes successfully and the returned value includes only the expected stdout content. This will make the intended stderr behavior explicit and protected by tests.

Suggested implementation:

```java
import static org.junit.jupiter.api.Assertions.assertTrue;
import static org.junit.jupiter.api.Assertions.assertNotNull;
import static org.junit.jupiter.api.Assertions.assertFalse;

import org.junit.jupiter.api.Test;

public class ProcessRunnerTest {

    @Test
    void runWithOutput_returnsCommandOutput() {
        String output = ProcessRunner.runWithOutput("echo", "hello");

        assertNotNull(output, "Output should not be null");
        assertFalse(output.isEmpty(), "Output should not be empty");
        assertTrue(output.contains("hello"), "Output should contain 'hello'");
    }

    @Test
    void runWithOutput_ignoresStderrAndReturnsOnlyStdout() {
        String output = ProcessRunner.runWithOutput(
                "sh",
                "-c",
                "echo stdout && echo stderr 1>&2"
        );

        assertNotNull(output, "Output should not be null");
        assertFalse(output.isEmpty(), "Output should not be empty");
        assertTrue(output.contains("stdout"), "Output should contain only stdout content");
        assertFalse(output.contains("stderr"), "Output should not contain stderr content");
    }
}

```

If your test environment does not guarantee a POSIX-compatible `sh` shell (e.g., pure Windows without WSL or Git Bash), you will need to adapt the command invocation in `runWithOutput_ignoresStderrAndReturnsOnlyStdout` to a platform-appropriate shell or helper script that can write to both stdout and stderr.
</issue_to_address>

Sourcery is free for open source - if you like our reviews please consider sharing them ✨
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.

Comment on lines +44 to +49
private static String readOutput(Process process) throws IOException {
// Read the output from the process
BufferedReader reader = new BufferedReader(new InputStreamReader(process.getInputStream()));
StringBuilder output = new StringBuilder();
String line;
while ((line = reader.readLine()) != null) {
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

suggestion (bug_risk): Close the BufferedReader/InputStream via try-with-resources to avoid leaking OS resources.

The BufferedReader (and underlying InputStream) remains open for the lifetime of the process, which can cause resource leaks in long-running or repeated executions. Wrap the reader in a try-with-resources (or close it in a finally block) so it’s always released:

try (BufferedReader reader = new BufferedReader(new InputStreamReader(process.getInputStream()))) {
    StringBuilder output = new StringBuilder();
    String line;
    while ((line = reader.readLine()) != null) {
        output.append(line).append("\n");
    }
    return output.toString();
}

Comment on lines +24 to +26
// Redirect output and error streams to a pipe
processBuilder.redirectOutput(ProcessBuilder.Redirect.PIPE);
processBuilder.redirectError(ProcessBuilder.Redirect.PIPE);
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

issue (bug_risk): Reading only stdout while piping stderr risks deadlock for verbose error output.

In runWithOutput, both stdout and stderr are piped but only stdout is read. If the subprocess writes heavily to stderr, its buffer can fill and block the process so waitFor() never returns. Consider either using redirectErrorStream(true) and reading a single stream, or consuming stderr concurrently (e.g., separate thread) / redirecting it to inheritIO or logging instead of a pipe.

public class ProcessRunnerTest {

@Test
void runWithOutput_returnsCommandOutput() {
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

suggestion (testing): Add a test for runWithOutput when the process exits with a non-zero status

Because waitForExit only logs a warning on non-zero exits and doesn’t throw, please add a test that runs a command exiting with status 1 and asserts that runWithOutput does not throw and returns the expected output. This will lock in the intended behavior for failure cases and prevent regressions.

Comment on lines +10 to +17
@Test
void runWithOutput_returnsCommandOutput() {
String output = ProcessRunner.runWithOutput("echo", "hello");

assertNotNull(output, "Output should not be null");
assertFalse(output.isEmpty(), "Output should not be empty");
assertTrue(output.contains("hello"), "Output should contain 'hello'");
}
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

suggestion (testing): The test command may not be fully portable across platforms

Using echo as the command makes this test non-portable (e.g., on Windows echo is usually a shell built-in, not an executable). To keep the test reliable across CI environments, consider using a known binary available in the test environment (such as java) or choosing the command based on the detected OS.

Suggested change
@Test
void runWithOutput_returnsCommandOutput() {
String output = ProcessRunner.runWithOutput("echo", "hello");
assertNotNull(output, "Output should not be null");
assertFalse(output.isEmpty(), "Output should not be empty");
assertTrue(output.contains("hello"), "Output should contain 'hello'");
}
@Test
void runWithOutput_returnsCommandOutput() {
String osName = System.getProperty("os.name").toLowerCase();
String output;
if (osName.contains("win")) {
// On Windows, echo is a cmd.exe built-in
output = ProcessRunner.runWithOutput("cmd.exe", "/c", "echo hello");
} else {
// On Unix-like systems, echo is typically a shell built-in
output = ProcessRunner.runWithOutput("sh", "-c", "echo hello");
}
assertNotNull(output, "Output should not be null");
assertFalse(output.isEmpty(), "Output should not be empty");
assertTrue(output.contains("hello"), "Output should contain 'hello'");
}


assertNotNull(output, "Output should not be null");
assertFalse(output.isEmpty(), "Output should not be empty");
assertTrue(output.contains("hello"), "Output should contain 'hello'");
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

suggestion (testing): Consider adding a test that covers stderr handling in runWithOutput

Currently we only cover a stdout-only command. Since runWithOutput pipes stderr but only reads stdout, please add a test where the child writes to both streams, and assert that it completes successfully and the returned value includes only the expected stdout content. This will make the intended stderr behavior explicit and protected by tests.

Suggested implementation:

import static org.junit.jupiter.api.Assertions.assertTrue;
import static org.junit.jupiter.api.Assertions.assertNotNull;
import static org.junit.jupiter.api.Assertions.assertFalse;

import org.junit.jupiter.api.Test;

public class ProcessRunnerTest {

    @Test
    void runWithOutput_returnsCommandOutput() {
        String output = ProcessRunner.runWithOutput("echo", "hello");

        assertNotNull(output, "Output should not be null");
        assertFalse(output.isEmpty(), "Output should not be empty");
        assertTrue(output.contains("hello"), "Output should contain 'hello'");
    }

    @Test
    void runWithOutput_ignoresStderrAndReturnsOnlyStdout() {
        String output = ProcessRunner.runWithOutput(
                "sh",
                "-c",
                "echo stdout && echo stderr 1>&2"
        );

        assertNotNull(output, "Output should not be null");
        assertFalse(output.isEmpty(), "Output should not be empty");
        assertTrue(output.contains("stdout"), "Output should contain only stdout content");
        assertFalse(output.contains("stderr"), "Output should not contain stderr content");
    }
}

If your test environment does not guarantee a POSIX-compatible sh shell (e.g., pure Windows without WSL or Git Bash), you will need to adapt the command invocation in runWithOutput_ignoresStderrAndReturnsOnlyStdout to a platform-appropriate shell or helper script that can write to both stdout and stderr.

@orpiske orpiske merged commit 7bdc916 into wanaku-ai:main Mar 3, 2026
5 checks passed
orpiske added a commit to orpiske/wanaku-capabilities-java-sdk that referenced this pull request Mar 3, 2026
- Use redirectErrorStream(true) to prevent stderr deadlock
- Use try-with-resources for BufferedReader in readOutput
- Restore interrupt status before rethrowing WanakuException
- Make tests cross-platform with OS detection
- Add test for non-zero exit status behavior
- Add test for merged stderr/stdout stream
orpiske added a commit that referenced this pull request Mar 3, 2026
- Use redirectErrorStream(true) to prevent stderr deadlock
- Use try-with-resources for BufferedReader in readOutput
- Restore interrupt status before rethrowing WanakuException
- Make tests cross-platform with OS detection
- Add test for non-zero exit status behavior
- Add test for merged stderr/stdout stream
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant